Change signature opts to include type, cleanup error handling

Previously we would pass the `verification-key` and `verification-file`
to all backends, ignoring errors from loading keys until we
found one that worked.

Instead, change the options to be `verification-<engine>-key`
and `verification-<engine>-file`, and then
rework this to use standard error handling; barf explicitly if
we can't load the public keys for example.  Preserve
the semantics of accepting the first valid signature.  The
first signature error is captured, the others are currently
compressed into a `(and %d more)` prefix.

And now that I look at this more closely there's a lot of
duplication between the two code paths in pull.c for verifying;
will dedup this next.

diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c
index 888fa1e71fcf778bd9a41282f5112ab1828da5d9..263f1cfcd5a07589e232c4fffb88363afe91e97d 100644 (file)
--- a/src/libostree/ostree-repo-pull.c
+++ b/src/libostree/ostree-repo-pull.c
@@ -1470,11 +1470,23 @@ process_gpg_verify_result (OtPullData            *pull_data,
 }
 #endif /* OSTREE_DISABLE_GPGME */
 
+static gboolean
+get_signapi_remote_option (OstreeRepo *repo,
+                           OstreeSign *sign,
+                           const char *remote_name,
+                           const char *keysuffix,
+                           char      **out_value,
+                           GError    **error)
+{
+  g_autofree char *key = g_strdup_printf ("verification-%s-%s", ostree_sign_get_name (sign), keysuffix);
+  return ostree_repo_get_remote_option (repo, remote_name, key, NULL, out_value, error);
+}
+
 /* _load_public_keys:
  *
  * Load public keys according remote's configuration:
- * inlined key passed via config option `verification-key` or
- * file name with public keys via `verification-file` option.
+ * inlined key passed via config option `verification-<signapi>-key` or
+ * file name with public keys via `verification-<signapi>-file` option.
  *
  * If both options are set then load all all public keys
  * both from file and inlined in config.
@@ -1493,15 +1505,10 @@ _load_public_keys (OstreeSign *sign,
   gboolean loaded_from_file = TRUE;
   gboolean loaded_inlined = TRUE;
 
-  ostree_repo_get_remote_option (repo,
-                                 remote_name,
-                                 "verification-file", NULL,
-                                 &pk_file, NULL);
-
-  ostree_repo_get_remote_option (repo,
-                                 remote_name,
-                                 "verification-key", NULL,
-                                 &pk_ascii, NULL);
+  if (!get_signapi_remote_option (repo, sign, remote_name, "file", &pk_file, error))
+    return FALSE;
+  if (!get_signapi_remote_option (repo, sign, remote_name, "key", &pk_ascii, error))
+    return FALSE;
 
   /* return TRUE if there is no configuration for remote */
   if ((pk_file == NULL) &&(pk_ascii == NULL))
@@ -1571,9 +1578,10 @@ _sign_verify_for_remote (OstreeRepo *repo,
 {
   /* list all signature types in detached metadata and check if signed by any? */
   g_auto (GStrv) names = ostree_sign_list_names();
-  g_autoptr (GError) verification_error = NULL;
-
-  glnx_throw (&verification_error, "signed with unknown key");
+  guint n_invalid_signatures = 0;
+  guint n_unknown_signatures = 0;
+  g_autoptr (GError) last_sig_error = NULL;
+  gboolean found_sig = FALSE;
 
   for (char **iter=names; iter && *iter; iter++)
     {
@@ -1581,10 +1589,12 @@ _sign_verify_for_remote (OstreeRepo *repo,
       g_autoptr (GVariant) signatures = NULL;
       const gchar *signature_key = NULL;
       GVariantType *signature_format = NULL;
-      g_autoptr (GError) local_error = NULL;
 
-      if ((sign = ostree_sign_get_by_name (*iter, &local_error)) == NULL)
-        continue;
+      if ((sign = ostree_sign_get_by_name (*iter, NULL)) == NULL)
+        {
+          n_unknown_signatures++;
+          continue;
+        }
 
       signature_key = ostree_sign_metadata_key (sign);
       signature_format = (GVariantType *) ostree_sign_metadata_format (sign);
@@ -1598,24 +1608,37 @@ _sign_verify_for_remote (OstreeRepo *repo,
         continue;
 
       /* Try to load public key(s) according remote's configuration */
-      if (_load_public_keys (sign, repo, remote_name, &local_error))
+      if (!_load_public_keys (sign, repo, remote_name, error))
+        return FALSE;
+
+      found_sig = TRUE;
+
+        /* Return true if any signature fit to pre-loaded public keys.
+          * If no keys configured -- then system configuration will be used */
+      if (!ostree_sign_data_verify (sign,
+                                    signed_data,
+                                    signatures,
+                                    last_sig_error ? NULL : &last_sig_error))
         {
-          /* Return true if any signature fit to pre-loaded public keys.
-           * If no keys configured -- then system configuration will be used */
-          if (ostree_sign_data_verify (sign,
-                                       signed_data,
-                                       signatures,
-                                       &local_error))
-            return TRUE;
+          n_invalid_signatures++;
+          continue;
         }
+      /* Accept the first valid signature */
+      return TRUE;
+    }
 
-      /* Save error message for better reason detection later if needed */
-      glnx_prefix_error (&verification_error, "%s", local_error->message);
+  if (!found_sig)
+    {
+      if (n_unknown_signatures > 0)
+        return glnx_throw (error, "No signatures found (%d unknown type)", n_unknown_signatures);
+      return glnx_throw (error, "No signatures found");
     }
 
-  /* In case if there were no signatures of known type
-   * or metadata contains invalid data */
-  return glnx_throw (error, "%s", verification_error->message);
+  g_assert (last_sig_error);
+  g_propagate_error (error, g_steal_pointer (&last_sig_error));
+  if (n_invalid_signatures > 1)
+    glnx_prefix_error (error, "(%d other invalid signatures)", n_invalid_signatures-1);
+  return FALSE;
 }
 
 static gboolean
@@ -2001,39 +2024,46 @@ scan_commit_object (OtPullData                 *pull_data,
   if (pull_data->sign_verify &&
       !g_hash_table_contains (pull_data->verified_commits, checksum))
     {
-      gboolean ret = FALSE;
-      g_autoptr (GError) verification_error = NULL;
+      g_autoptr(GError) last_verification_error = NULL;
+      gboolean found_any_signature = FALSE;
+      gboolean found_valid_signature = FALSE;
 
       /* list all signature types in detached metadata and check if signed by any? */
       g_auto (GStrv) names = ostree_sign_list_names();
-      for (char **iter=names; !ret && iter && *iter; iter++)
+      for (char **iter=names; iter && *iter; iter++)
         {
           g_autoptr (OstreeSign) sign = NULL;
-          g_autoptr (GError) local_error = NULL;
 
-          if ((sign = ostree_sign_get_by_name (*iter, &local_error)) == NULL)
+          if ((sign = ostree_sign_get_by_name (*iter, NULL)) == NULL)
             continue;
 
           /* Try to load public key(s) according remote's configuration */
-          if (_load_public_keys (sign, pull_data->repo, pull_data->remote_name, &local_error))
-            {
+          if (!_load_public_keys (sign, pull_data->repo, pull_data->remote_name, error))
+            return FALSE;
 
-              /* Set return to true if any sign fit */
-              if (ostree_sign_commit_verify (sign,
-                                             pull_data->repo,
-                                             checksum,
-                                             cancellable,
-                                             &local_error))
-                ret = TRUE;
-            }
+          found_any_signature = TRUE;
 
-          /* Save error message for better reason detection later if needed */
-          if (!ret)
-            glnx_prefix_error (&verification_error, "%s", local_error->message);
+          /* Set return to true if any sign fit */
+          if (ostree_sign_commit_verify (sign,
+                                          pull_data->repo,
+                                          checksum,
+                                          cancellable,
+                                          last_verification_error ? NULL : &last_verification_error))
+            {
+              found_valid_signature = TRUE;
+              break;
+            }
          }
 
-      if (!ret)
-        return glnx_throw (error, "Can't verify commit %s: %s", checksum, verification_error->message);
+      if (!found_any_signature)
+        return glnx_throw (error, "No signatures found for commit %s", checksum);
+
+      if (!found_valid_signature)
+        {
+          g_assert (last_verification_error);
+          g_propagate_error (error, g_steal_pointer (&last_verification_error));
+          return glnx_prefix_error (error, "Can't verify commit %s", checksum);
+        }
     }
 
   /* If we found a legacy transaction flag, assume we have to scan.

diff --git a/src/libostree/ostree-sign-ed25519.c b/src/libostree/ostree-sign-ed25519.c
index 8df61aedc19209f50a159182e13968d6fafefd3e..4d984d1e3bfc75afce9d28b90b9566a453bf5a87 100644 (file)
--- a/src/libostree/ostree-sign-ed25519.c
+++ b/src/libostree/ostree-sign-ed25519.c
@@ -242,7 +242,7 @@ gboolean ostree_sign_ed25519_data_verify (OstreeSign *self,
         }
     }
 
-  return glnx_throw (error, "Not able to verify: no valid signatures found");
+  return glnx_throw (error, "no valid ed25519 signatures found");
 #endif /* HAVE_LIBSODIUM */
 
   return FALSE;

diff --git a/tests/test-local-pull.sh b/tests/test-local-pull.sh
index 2b7ca13a5e4394a9a31e98a00cd825b1577b48aa..8cbc97328e39c5fc5c02f71d506ecad411e6c8d2 100755 (executable)
--- a/tests/test-local-pull.sh
+++ b/tests/test-local-pull.sh
@@ -121,27 +121,29 @@ if has_libsodium; then
 
     mkdir repo8
     ostree_repo_init repo8 --mode="archive"
-    ${CMD_PREFIX} ostree --repo=repo8 remote add --set=verification-key="${ED25519PUBLIC}" origin repo
+    ${CMD_PREFIX} ostree --repo=repo8 remote add --set=verification-ed25519-key="${ED25519PUBLIC}" origin repo
     cat repo8/config
 
-    if ${CMD_PREFIX} ostree --repo=repo8 pull-local --remote=origin --sign-verify repo test2 2>&1; then
+    if ${CMD_PREFIX} ostree --repo=repo8 pull-local --remote=origin --sign-verify repo test2 2>err.txt; then
         assert_not_reached "Ed25519 signature verification unexpectedly succeeded"
     fi
+    assert_file_has_content err.txt 'ed25519: commit have no signatures of my type'
     echo "ok --sign-verify with no signature"
 
     ${OSTREE} sign test2 ${ED25519SECRET}
 
     mkdir repo9
     ostree_repo_init repo9 --mode="archive"
-    ${CMD_PREFIX} ostree --repo=repo9 remote add --set=verification-key="$(gen_ed25519_random_public)" origin repo
-    if ${CMD_PREFIX} ostree --repo=repo9 pull-local --remote=origin --sign-verify repo test2 2>&1; then
+    ${CMD_PREFIX} ostree --repo=repo9 remote add --set=verification-ed25519-key="$(gen_ed25519_random_public)" origin repo
+    if ${CMD_PREFIX} ostree --repo=repo9 pull-local --remote=origin --sign-verify repo test2 2>err.txt; then
         assert_not_reached "Ed25519 signature verification unexpectedly succeeded"
     fi
+    assert_file_has_content err.txt 'no valid ed25519 signatures found'
     echo "ok --sign-verify with wrong signature"
 
     mkdir repo10
     ostree_repo_init repo10 --mode="archive"
-    ${CMD_PREFIX} ostree --repo=repo10 remote add --set=verification-key="${ED25519PUBLIC}" origin repo
+    ${CMD_PREFIX} ostree --repo=repo10 remote add --set=verification-ed25519-key="${ED25519PUBLIC}" origin repo
     ${CMD_PREFIX} ostree --repo=repo10 pull-local --remote=origin --sign-verify repo test2
     echo "ok --sign-verify"
 else

diff --git a/tests/test-signed-pull-summary.sh b/tests/test-signed-pull-summary.sh
index c328d2881252401ed32a0f5f1b764fd536868494..7a7dd073c0232c5196b32948587297b684cfa735 100755 (executable)
--- a/tests/test-signed-pull-summary.sh
+++ b/tests/test-signed-pull-summary.sh
@@ -115,7 +115,7 @@ do
     ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN}
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     ${OSTREE} --repo=repo pull origin main
     assert_has_file repo/tmp/cache/summaries/origin
     assert_has_file repo/tmp/cache/summaries/origin.sig
@@ -136,7 +136,7 @@ do
     echo "ok ${engine} prune summary cache"
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     mkdir cachedir
     ${OSTREE} --repo=repo pull --cache-dir=cachedir origin main
     assert_not_has_file repo/tmp/cache/summaries/origin
@@ -152,13 +152,13 @@ do
     echo "ok ${engine} pull with signed summary and cachedir"
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{,.good}
     echo invalid > ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig
     if ${OSTREE} --repo=repo pull origin main 2>err.txt; then
         assert_not_reached "Successful pull with invalid ${engine} signature"
     fi
-    assert_file_has_content err.txt "signed with unknown key"
+    assert_file_has_content err.txt "No signatures found"
     mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.good,}
     echo "ok ${engine} pull with invalid ${engine} summary signature fails"
 
@@ -167,7 +167,7 @@ do
     ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN}
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     ${OSTREE} --repo=repo pull origin main
     echo "ok ${engine} pull delta with signed summary"
 
@@ -212,7 +212,7 @@ cd ${test_tmpdir}
 # Reset to the old valid summary and pull to cache it
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.1,}
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.1,}
-repo_reinit --set=verification-key=${PUBLIC_KEY}
+repo_reinit --set=verification-ed25519-key=${PUBLIC_KEY}
 ${OSTREE} --repo=repo pull origin main
 assert_has_file repo/tmp/cache/summaries/origin
 assert_has_file repo/tmp/cache/summaries/origin.sig
@@ -226,7 +226,7 @@ cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.2,}
 if ${OSTREE} --repo=repo pull origin main 2>err.txt; then
     assert_not_reached "Successful pull with old summary"
 fi
-assert_file_has_content err.txt "signed with unknown key"
+assert_file_has_content err.txt "no valid ed25519 signatures found"
 assert_has_file repo/tmp/cache/summaries/origin
 assert_has_file repo/tmp/cache/summaries/origin.sig
 cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2
@@ -246,7 +246,7 @@ echo "ok ${engine} pull with signed summary remote old summary"
 # Reset to the old valid summary and pull to cache it
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.1,}
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.1,}
-repo_reinit --set=verification-key=${PUBLIC_KEY}
+repo_reinit --set=verification-ed25519-key=${PUBLIC_KEY}
 ${OSTREE} --repo=repo pull origin main
 assert_has_file repo/tmp/cache/summaries/origin
 assert_has_file repo/tmp/cache/summaries/origin.sig

diff --git a/tests/test-signed-pull.sh b/tests/test-signed-pull.sh
index 6d1afe2918e6ca5b00d04797c4d836d94a2f25e7..a8d52bc537ba17f70138714a10e81c1cc0847cf9 100755 (executable)
--- a/tests/test-signed-pull.sh
+++ b/tests/test-signed-pull.sh
@@ -80,22 +80,22 @@ if ${CMD_PREFIX} ostree --repo=repo pull origin main; then
 fi
 echo "ok pull failure without keys preloaded"
 
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-key "somewrongkey"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-dummy-key "somewrongkey"
 if ${CMD_PREFIX} ostree --repo=repo pull origin main; then
     assert_not_reached "pull with unknown key unexpectedly succeeded"
 fi
 echo "ok pull failure with incorrect key option"
 
-${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-key
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-file "/non/existing/file"
+${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-dummy-key
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-dummy-file "/non/existing/file"
 if ${CMD_PREFIX} ostree --repo=repo pull origin main; then
     assert_not_reached "pull with unknown keys file unexpectedly succeeded"
 fi
 echo "ok pull failure with incorrect keys file option"
 
 # Test with correct dummy key
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-key "${DUMMYSIGN}"
-${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-file
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-dummy-key "${DUMMYSIGN}"
+${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-dummy-file
 test_signed_pull "dummy" ""
 
 if ! has_libsodium; then
@@ -117,7 +117,7 @@ SECRET=${ED25519SECRET}
 COMMIT_ARGS="--sign=${SECRET} --sign-type=ed25519"
 
 repo_init --set=sign-verify=true
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-key "${PUBLIC}"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-ed25519-key "${PUBLIC}"
 test_signed_pull "ed25519" "key"
 
 # Prepare files with public ed25519 signatures
@@ -130,13 +130,13 @@ for((i=0;i<100;i++)); do
 done > ${PUBKEYS}
 
 # Test case with the file containing incorrect signatures and with the correct key set
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-file "${PUBKEYS}"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-ed25519-file "${PUBKEYS}"
 test_signed_pull "ed25519" "key+file"
 
 # Add correct key into the list
 echo ${PUBLIC} >> ${PUBKEYS}
 
 repo_init --set=sign-verify=true
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-file "${PUBKEYS}"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-ed25519-file "${PUBKEYS}"
 test_signed_pull "ed25519" "file"